-
Notifications
You must be signed in to change notification settings - Fork 165
[Terra-Dropdown] VO does not announce Expanded state and Index of list items while navigating through arrow keys #3805
Conversation
|
…6586/Dropdown-announce-index
@@ -69,6 +78,7 @@ class DropdownList extends React.Component { | |||
event.preventDefault(); | |||
} else if (keyCode === KeyCode.KEY_DOWN) { | |||
if (!this.pressed) { | |||
this.expanded = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a unit test to see if this gets set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented.
@@ -1,6 +1,8 @@ | |||
# Changelog | |||
|
|||
## Unreleased | |||
* Added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Added | |
* Added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there are 2 Added
headings and they can be combined together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
const ofText = this.props.intl.formatMessage({ id: 'Terra.dropdownButton.of' }); | ||
const totalItems = this.props.children.length; | ||
let ariaLabel = null; | ||
if (SharedUtil.isMac() && currentIndex === 1 && totalItems) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we will be removing aria-expanded on DropdownButton.jsx . we should remove this condition of MAC and make it consistent across all the screen readers and browsers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented and tested it is working as expected.
if (SharedUtil.isMac() && currentIndex === 1 && totalItems) { | ||
ariaLabel = `${this.expanded} ${currentItemLabel} (${currentIndex} ${ofText} ${totalItems})`; | ||
} else if (SharedUtil.isMac() && currentIndex !== 1 && totalItems) { | ||
ariaLabel = `${currentItemLabel} (${currentIndex} ${ofText} ${totalItems})`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ariaLabel = `${currentItemLabel} (${currentIndex} ${ofText} ${totalItems})`; | |
ariaLabel = `${currentItemLabel}, (${currentIndex} ${ofText} ${totalItems})`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
it('should set the aria-label property to empty on keydown on Mac', () => { | ||
// Set the mock implementation for isMac | ||
SharedUtil.isMac.mockReturnValue(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on the mock
expect(firstListItemAriaLabelValue).toEqual(expectedAriaLabelValueInitial); | ||
|
||
// Simulate keydown event | ||
wrapper.instance().handleKeyDown(eventMock); | ||
const updatedFirstListItemAriaLabelValue = wrapper.find('#firstItem').props()['aria-label']; | ||
const expectedAriaLabelValue = '1st Option,(1 of 3)'; | ||
expect(updatedFirstListItemAriaLabelValue).toEqual(expectedAriaLabelValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put all the expect statements at the bottom for better readability.
expect(firstListItemAriaLabelValue).toEqual(expectedAriaLabelValueInitial); | |
// Simulate keydown event | |
wrapper.instance().handleKeyDown(eventMock); | |
const updatedFirstListItemAriaLabelValue = wrapper.find('#firstItem').props()['aria-label']; | |
const expectedAriaLabelValue = '1st Option,(1 of 3)'; | |
expect(updatedFirstListItemAriaLabelValue).toEqual(expectedAriaLabelValue); | |
// Simulate keydown event | |
wrapper.instance().handleKeyDown(eventMock); | |
const updatedFirstListItemAriaLabelValue = wrapper.find('#firstItem').props()['aria-label']; | |
const expectedAriaLabelValue = '1st Option,(1 of 3)'; | |
expect(firstListItemAriaLabelValue).toEqual(expectedAriaLabelValueInitial); | |
expect(updatedFirstListItemAriaLabelValue).toEqual(expectedAriaLabelValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented as suggested.
wrapper.instance().listRef = listRefMock; | ||
const firstListItem = wrapper.find('#firstItem'); | ||
const firstListItemAriaLabelValue = firstListItem.props()['aria-label']; | ||
const expectedAriaLabelValueInitial = `${translationsFile['Terra.dropdownButton.expanded']}1st Option,(1 of 3)`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this to expectedAriaLabelInitialValue
const expectedAriaLabelValueInitial = `${translationsFile['Terra.dropdownButton.expanded']}1st Option,(1 of 3)`; | |
const expectedAriaLabelInitialValue = `${translationsFile['Terra.dropdownButton.expanded']}1st Option,(1 of 3)`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@@ -89,4 +115,58 @@ describe('Dropdown List', () => { | |||
); | |||
expect(wrapper).toMatchSnapshot(); | |||
}); | |||
|
|||
it('should set the aria-label property to empty on keydown on Mac', () => { | |||
// Set the mock implementation for isMac |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
// Set the mock implementation for isMac | |
// Sets the mock return value for isMac() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
…6586/Dropdown-announce-index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was about to approve until I noticed this one last thing!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we don't have the translations at this time, we should only keep en.json
and en-US.json
. The remaining files will be provided by the translations team when they are ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have received translations from translation team and I have added them in the translation files . TRANS-14545
Sorry, I was about to approve until I noticed this one last thing!! |
|
let ariaLabel = null; | ||
if (SharedUtil.isMac()) { | ||
if (currentIndex === 1 && totalItems) { | ||
ariaLabel = `${this.expanded}${activeOption}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
ariaLabel = `${this.expanded}${activeOption}`; | |
ariaLabel = `${this.expanded}, ${activeOption}`; |
Lets remove the special characters like dot
and comma
from translations and include it as part of strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ariaLabel = `${this.expanded}${currentItemLabel}`; | ||
} else if (currentIndex !== 1 && totalItems) { | ||
ariaLabel = `${currentItemLabel}`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we simplify this :
const activeOption = (SharedUtil.isMac()) ? this.props.intl.formatMessage({ id: 'Terra.dropdownButton.activeOption' }, { currentItemLabel, currentIndex, totalItems }) : currentItemLabel;
let ariaLabel = '';
if (currentIndex === 1 && totalItems) {
ariaLabel = `${this.expanded}, ${activeOption}`;
} else if (currentIndex !== 1 && totalItems) {
ariaLabel = activeOption;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified for better readability.
@@ -1,4 +1,4 @@ | |||
{ | |||
"Terra.dropdownButton.moreOptions": "More Options", | |||
"Terra.dropdownButton.selected": "Valittu." | |||
"Terra.dropdownButton.selected": "Valittu" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are missing translations here !! Please make sure translations are added from valid file it should be from fi-FI.json file provided by translation team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't received translations for fi-FI.json when I checked with the translation team she told it is not a supported locale.
Summary
[Terra-Dropdown] VO does not announce Expand/Collapse state and Index of list items while navigating through arrow keys
What was changed:
Added screenreader support to announce index and Expanded state for dropdown list items by adding aria-label property.
Expand.collapse_VOL.mov
Untitled.2.mp4
Untitled.1.mp4
This PR resolves:
UXPLATFORM-9058
Thank you for contributing to Terra.
@cerner/terra